Repository navigation
feat(dpp)!: check document schemas natively at registration from protocol version 15 - #5538
Conversation
From protocol version 15, document validation checks a document's properties against its type's typed validation schema with Platform's own code, in place of the `jsonschema` crate. `validate_document` 2, selected by a new `CONTRACT_VERSIONS_V7` that only protocol version 15 uses, is generation 1 with the schema step swapped. Failures are still `JsonSchemaError`s, so a failed transition's code and fees are unchanged; only the error's message and paths, which the results hash does not cover, are worded by the new code. `DocumentSchemaValidator` repeats the pinned fork's checks keyword by keyword, with the crates the fork uses: `num-cmp` for exact number comparisons, `fraction` for `multipleOf` when the quotient overflows, `time` for dates, `url` for URIs. `enum`, `const` and `uniqueItems` compare as the fork does (numbers by value up to 15 array elements, by JSON value above), lengths count characters, patterns are converted and compiled once per document type, `byteArray` refuses what the fork's custom keyword refuses, and `contains` asks only whether an element matches. A cycle of `$ref`s that consumes nothing of the value returns an internal error instead of recursing. The validator is built on first use and kept beside the `jsonschema` one on the document type. A document type whose schema the builder refuses keeps generation 1's check, so no contract registered earlier is left without one. Tests: fixed cases for each number, array, string, format, object and reference rule, checked against the existing validator; a generated differential that builds the native and existing validators for 800 generated schemas and compares their verdicts on 34 documents each; protocol version 15 against 14 on a registered contract (same verdict, same first error code); the fallback for a schema with `not`; and a `$ref` cycle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…otocol version 15 Protocol version 15 selects parser generation 4 (`try_from_schema` 4 in `CONTRACT_VERSIONS_V7`): generation 3's parse, with full validation checking the enriched schema natively instead of against the document meta-schema through the `jsonschema` crate, which also stops compiling it. Each new document type must then have a typed validation schema, which its documents are checked against natively. This commit ports the root shape of a document schema and the rules of property schemas, at every level and in `$defs`, and of typed array element schemas. A schema breaking one is refused with the `JsonSchemaError` the meta-schema check returned, so the error code stays. The native checks read the JSON the meta-schema check read: a repeated key's last entry, and the crate's readings of integers, bounds, `const` numbers and unique lists. The validation schema builder also admits `$` in a plain-name `$id`, as `refersTo` data carries (`"$id": "$ownerId"`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…yte names Generation 4 checks each index as the document meta-schema did: its closed key set, `name`, `properties` (each one property, `asc` only), the flags, `contested`, the count, sum and average keywords, `ranked*`, `timeRange`, `integerRange`, `terminal`, `skipIfAbsent` and `summableOffCountIndex`, with the meta-schema's conditions between them. One rule differs: an index property name is at most 255 bytes, the longest key GroveDB stores, where the meta-schema admitted 256 characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nt grammars Generation 4 checks the document-type keywords as the document meta-schema did (the flags, `canBeDeleted`, the security and restriction modes, `documentsSummable` and `documentsAverageable`, `moderatorAbilities`, `ttl`, `entryPayload`, `actionFees`, `tokenCost`, `transient`, `immutable`, `retractedWhen` and their conditions), and the grammars the meta-schema held: `refersTo` on properties, on array elements and as `ownerRefersTo` or `creatorRefersTo`, with its `findBy`, `where` and `constraints`, and the rule language of `propertyConstraints` and `deleteConstraints`. With this, every rule of the meta-schema is checked natively, and a schema breaking one is refused with a `JsonSchemaError` as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-schema The native checks are compared with the document meta-schema, and generation 4's parse with generation 3's, on: - every document type of the contract corpus (1,503), which both generations parse to the same document type; - hand-written document types using the keywords the corpus uses little (references, encryption, fees, token costs, moderation, constraints, windows and ranked indexes, index-only types); - schemas generated by changing those at random places (2,700) and the corpus types (3,006). Every case gets the same verdict from both sides. A refusal keeps its error code, except where the `jsonschema` compile refused a schema the meta-schema admitted, which the validation schema builder now refuses, and the 255-byte index property name cap. Targeted tests pin each rule area, where an error is reported, and the cap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit 782ec00. Normal review starts when eligible; priority review starts as soon as a slot is available. |
Both sides introduced `CONTRACT_VERSIONS_V7` for protocol version 15: v5.1-dev with `parse_property_constraints` 1 (byte array aggregate keys, DPNS v3), this branch with `validate_document` 2 (native document validation). The merged table is V6 with both changes, written as v5.1-dev wrote it (V6 updated by struct update), and `PLATFORM_V15` gains note 5 for native document validation beside note 4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in v5.1-dev through the base branch, including DPNS v3, whose `CONTRACT_VERSIONS_V7` (V6 with `parse_property_constraints` 1, written by struct update) the base merged with `validate_document` 2. The merged table also sets `try_from_schema` 4, overriding V6's class method versions the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n 15's changes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`StatelessJsonSchemaLazyValidator` derived `Debug`, so adding the native validator's cache slot added `, OnceLock(<uninit>)` to every document type's debug output. The drive-abci nullifier batch guard pins a hash of a validation result's debug output at the latest protocol version, and failed although actions, errors, fees, operations and state were unchanged. The validator now prints the `jsonschema` validator alone, exactly as the derived impl did before: the native validator is a cache, which `PartialEq` and `Clone` leave out too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in the validator debug fix: a document type prints its validators as before the native one was added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of head 7991a1d found no blocking correctness or released-protocol compatibility defect. The remaining findings concern the shared validation-algorithm flag, an incorrect nested error location, undocumented structured-error changes, and a redundant numeric comparison branch. Current PR checks pass, including Kotlin SDK build/tests; no dedicated DPP Rust test result is listed, and no local builds or tests were run.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to protocol-version-gated document schema validation that directly alters consensus-critical contract registration behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs:225-229: Keep validation algorithm selection at a versioned boundary
Generation 4 calls `try_from_schema_generation_3` with this flag set to true, selecting both native schema validation and the typed-validation-schema requirement inside code also reached by historical generations. Existing callers pass false, so this does not demonstrate a released-protocol regression. However, it violates the book's “Shared helpers get a versioned method, not a capability flag” rule: this selects an algorithm rather than supplying a generation-owned grammar constant. Give generation 4 its own implementation with unconditional native checks, or version the shared validation helper through an `OptionalFeatureVersion` table entry. Either approach preserves the existing protocol-15 activation while making the compatibility boundary structural.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/properties.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/properties.rs:299-302: Include the properties segment in nested-position errors
Here `self` identifies the containing object schema, not its `properties` map. Removing `position` from the existing fixture's `meta.properties.a` therefore produces `/properties/meta/a` instead of `/properties/meta/properties/a`, pointing clients at a nonexistent location. Enter the `properties` segment before the member name. Extend the existing missing-position test to assert the path: the differential harness currently treats any pair of refusals as equivalent, so it cannot detect this defect.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs:123-141: Preserve or disclose changes to structured schema errors
The native errors differ from the old errors beyond message text. `fail_with` always supplies an empty `schema_path`, type failures omit the previous `{type: ...}` parameter, and `name_error` omits the nested property-name diagnostic parameters. Native `oneOf` failures also use keyword `oneOf`, whereas the existing conversion in `error_data.rs` emits `oneOfNotValid` when no branch matches. These fields are exposed by `JsonSchemaError`, so consumers inspecting structured diagnostics observe a change despite the PR's “Only the message text differs” claim. Preserve the intended fields, or explicitly document these protocol-15 diagnostic changes and add focused assertions for the supported error shape.
- [NITPICK] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs:568-574: Remove the redundant branch in numeric equality
`right_integer` is the unchanged `right` value, so both branches perform exactly `left == right`. The fractional-part check has no effect and suggests an integer-specific comparison that does not exist. Collapse the arm to the comparison it actually performs.
…e field The choice between the document meta-schema check and the native one was a `native_schema_checks` flag on the shared `ParserGeneration` context, set by a parser generation 4 that otherwise ran generation 3. The coding conventions ask for a versioned method there instead: the flag moved the version decision out of the tables. `DocumentTypeSchemaVersions` gains `check_document_type_schema`: 0 (every shipped table) checks a schema against the meta-schema through the `jsonschema` crate and compiles it, as before; 1 (`CONTRACT_VERSIONS_V7`, protocol version 15) checks it natively and requires a typed validation schema. The shared `validate_document_type_schema` and the new `require_validation_schema`, run last in generation 3's parse, dispatch on it. Generation 4, the flag and the parse parameter are gone, and protocol version 15 keeps `try_from_schema` 3. The comparison tests register each case at the latest protocol version with the field at 1 and at 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The native check named the member of an object's `properties` that has no `position` from the object itself, `/properties/meta/a`, where the meta-schema check reported `/properties/meta/properties/a`. It now enters `properties` first, and a test pins the path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and parameters
The native checks kept the old error code but not every structured
field of the `JsonSchemaError`. They now report as the meta-schema check
did:
- `type` with its `{type}` parameter (several types in sorted order);
- a closed property schema as `unevaluatedProperties` with
`{unexpected}`, and `additionalProperties` without a property name;
- `oneOfNotValid` for a `oneOf` no branch of which matches;
- `not` with the schema it negates, and the element reference check at
`refersTo/type`;
- a property name's error at the object that holds it, with the name's
own error nested as `{instancePath, instance, params, keyword,
propertyName}`;
- the meta-schema's exact pattern in every `pattern` error, and a
token contract id byte as `type` or `maximum`.
A test pins keyword, location, parameters and property name against the
meta-schema check on 36 single-rule violations. The `schema_path` stays
empty and the nested `schemaPath` absent: the meta-schema location has
no native counterpart. The redundant branch of the numeric comparison
is also gone.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…or did The native document validator kept the error code but not every structured field of the `JsonSchemaError`. It now reports as the `jsonschema` validator does: - a `$ref` target's violations without a `$ref` schema path segment, as if the target sat in the place of the reference; - a `dependentRequired` miss as `required` under `/dependentRequired`; - `byteArray` violations without a schema path; - the types of a union in alphabetical order; - `additionalProperties` without a property name. A test pins keyword, instance path, schema path, parameters and property name against the `jsonschema` validator on 30 single-rule violations. A `contains` candidate is checked without building diagnostics: a walk that only asks whether a value matches records a failure flag, and a violation's parameters and message are built only when reported. Error parameters convert schema values without cloning them. The document validation tests now assert that protocol version 15 builds and uses the native validator, that 14 never builds it, and that a schema without a validation schema records that it has none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in the native document validator's review fixes: violations reported as the jsonschema validator reports them, match-only checks without diagnostics, and activation assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lk paths Two more error shapes now match the `jsonschema` validator: - a `byteArray` violation reached through `$ref`s carries the path of the reference sites, which the validator adds as it returns through each `$ref`, where an unreferenced one still carries none; - a `type` list of one type reports that type, not a list of it. The structured comparison test gains both, through one and two references and inside a referenced object. The walk keeps its instance and schema paths as borrowed names and indices, rendered only when a violation is reported, and pushes a typed array's `items` segment once for all its elements. `uniqueItems` above 15 elements groups the elements by hash in one sorted vector instead of a vector per element, with the same equality deciding within a group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in the native document validator's second round of review fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… declared The native validator looked each key of an object up among the declared properties to find unexpected ones, even when there were none: 5,050 name comparisons for a valid object of 100 properties. The traversal before it already looks each declared property up in the object, so it now counts the declared keys the object holds. A declared name occurs once, so when the count equals the object's size there is no other key, and the search runs only when there is one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in the additionalProperties key-search skip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At b13a8be, the native registration checks are isolated to protocol version 15, and no blocking defect was confirmed. Three prior findings are fixed; one non-blocking structured-error compatibility gap remains. This was a static review with no builds or tests run; current CI checks pass but provide no targeted Rust/DPP test result.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large native_schema implementation, particularly check_document_type_schema, replaces consensus-relevant contract registration acceptance rules at protocol version 15 with intricate native validation logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/indexes.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/indexes.rs:79-82: Preserve or disclose changes to structured schema errors
The structured-error corrections leave this exclusion branch returning different parameters. An otherwise valid document schema containing an index such as `{"name":"i","summableOffCountIndex":"base","rangeSummable":true,"terminal":"name"}` reaches `fail("not", ...)`, which supplies `{}`. The meta-schema's corresponding `then.not` condition instead produces `params.not = {"anyOf":[{"required":["terminal"]},{"required":["summable"]},{"required":["averageable"]}]}`: `error_data.rs` explicitly preserves the negated schema. This is an observable parameter difference beyond the message and schema-path exceptions disclosed in the PR description. Use the existing `not_error` helper here and add this single-rule violation to the focused error-shape comparison, or explicitly disclose the additional parameter difference.
The refusal of a `terminal`, `summable` or `averageable` beside `summableOffCountIndex` kept `not` with empty parameters, where the meta-schema check gives the negated schema. It now uses `not_error` like the other `not` refusals, and the error-shape comparison gains the case (37 single-rule violations). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 23c99b2, protocol-15 activation remains isolated from shipped versions, and all five prior findings are fixed; no blocking issue remains. One non-blocking signed-zero uniqueness difference needs focused coverage and disclosure. This was a static review: current GitHub checks pass, but no dedicated DPP Rust test result is listed, and no builds or tests were run locally.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large and intricate change that directly alters protocol-versioned consensus validation rules for data-contract registration, including native schema acceptance and rejection behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs:482-486: Cover and disclose the signed-zero uniqueness difference
The reused `is_unique` helper differs from the legacy meta-schema check above the 15-element threshold. In the pinned `dashpay/jsonschema-rs` revision `aacc1ab5140d`, `unique_items.rs` hashes floats using their unchanged `to_bits()`, whereas the native helper normalizes both signed zeros before hashing. Consequently, an otherwise valid numeric property's 16-element enum `[0.0, -0.0, 1, 2, ..., 14]` can pass the legacy check but is rejected here with `uniqueItems`. The validating JSON conversion preserves signed zero, and the typed schema builder can represent this enum, so the existing documented exceptions do not cover the difference. Add focused 15/16-element boundary fixtures and disclose this stricter protocol-15 behavior in the module documentation and PR description. Keep the native helper's correct zero normalization rather than reproducing the fork's hash/equality defect; any upstream correction belongs in `dashpay/jsonschema-rs`.
v5.1-dev's tradeable tokens (#5524) added note 5 to `PLATFORM_V15`'s notes, as this branch did for native document validation. Tradeable tokens keep note 5; native document validation becomes note 6, and the contract versions comment points to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in v5.1-dev's tradeable tokens through the base branch, which renumbered native document validation to note 6 of `PLATFORM_V15`. Native registration checks become note 7, and both the contract versions comment and note 7's cross-reference follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`uniqueItems` in the native document validator compares numbers by value at every list length, so `0.0` and `-0.0` are one value. This is a deliberate rule of protocol version 15; the doc comment now says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Brings in the uniqueness rule's doc comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # packages/rs-dpp/src/data_contract/document_type/schema/validation_schema/validate/mod.rs
…search `last_wins` searched the entries gathered so far for every key of a schema map. It now indexes where each key's first entry sits, keeping the first key's place and the last entry's value as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The native protocol-15 schema checker has two correctness defects: it accepts property schemas whose const value is not true, and it skips validating schemas attached to system properties even though the meta-schema validates those schemas. A separate upgrade-compatibility issue remains for protocol-14 contracts that are valid under the legacy validator but cannot be represented by the protocol-15 native validator; the current contract census does not prevent such contracts from being registered before activation.
🔴 3 blocking | 🟡 3 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to consensus-relevant document-contract validation and protocol-version dispatch, directly changing which schemas registrations accept at protocol version 15. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/properties.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/properties.rs:114-118: Enforce the meta-schema's const=true restriction
The v3 meta-schema defines the `const` keyword in a property schema as `"const": true`, so a schema such as `{"type":"number","const":10,"position":0}` must be rejected during registration. The combined `contains | const` arm accepts every JSON value instead, causing protocol-15 native registration to admit a schema that the protocol-14 meta-schema rejects. Keep `contains` unconstrained as intended, but validate `const` with the existing constant helper.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/document_type.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/document_type.rs:201-216: Validate schemas attached to system properties
`root_properties` skips all validation for entries named `$id`, `$ownerId`, `$revision`, and the other system properties. The meta-schema allows those names, but still applies the document-schema definition to each associated schema; for example, a system property schema containing an unknown keyword is rejected by the legacy meta-schema's closed schema rules. The native checker therefore accepts invalid protocol-15 registrations and does not preserve the claimed meta-schema verdict equivalence. Apply `property_schema` to system-property values while retaining the special name exception.
In `packages/rs-platform-version/src/version/system_limits/mod.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/system_limits/mod.rs:25-29: Disclose the new SystemLimits field in the in-place-changes note
`max_index_property_name_bytes` is backfilled into every shipped `SystemLimits` table, including the table selected by protocol 14. This is consensus-safe because historical `check_document_type_schema: 0` paths never read the field, but the PR's required in-place-change notes do not mention the addition. Document that the field is set to 255 in the historical tables and is read only by native schema-check version 1.
In `packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs:34-38: Preserve legacy document validation for contracts registered before protocol 15
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891896)
Protocol 15 selects the native document validator and typed validation-schema builder for every loaded contract, but stored contracts do not retain the validator generation used when they were registered. A contract registered under protocol 14 can therefore pass the legacy meta-schema and JSON Schema compilation while containing a schema the native builder cannot represent, such as a property with both `$ref` and `maxBytes`. After activation, the contract loads without full schema validation, but every document create or replace lazily builds the native validator and returns `InvalidContractStructure`, disabling document operations for that contract. The current corpus census only describes contracts known today and cannot protect a contract registered during the remaining protocol-14 window. Preserve the registration-era validator semantics or add an upgrade migration/compatibility gate before enabling the native validator globally.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/tests.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/tests.rs:742-753: Cover and disclose the signed-zero uniqueness difference
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891902)
The protocol-15 signed-zero rule is now disclosed, but the native registration tests still do not contain a `0.0`/`-0.0` pair or exercise both sides of the 15-element uniqueness threshold. The shared helper uses pairwise comparison through 15 items and a hashed path above that threshold, so a 16-element enum containing both signed-zero representations is the important regression case. Add focused 15- and 16-element registration fixtures asserting the intended protocol-15 refusal.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs:510-515: Cover and disclose the signed-zero uniqueness difference
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4353307235)
`Checker::unique` delegates registration decisions to `is_unique`, whose behavior changes from pairwise comparison to hashed uniqueness at 16 items. Although signed-zero normalization is intentional and documented, no direct native-check test protects that behavior across the boundary. Add focused enum or unique-list fixtures containing both `0.0` and `-0.0` at 15 and 16 items so the protocol-15 rule cannot silently regress.
…ks leave too A property's `const`, which the document meta-schema reads through the schema `true`, takes any value, and a system property's schema is not checked at all: the meta-schema check and the native checks admit a number `const`, and a `$createdAt` schema with an unknown keyword or a malformed bound, alike. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The native schema checker is substantially version-gated and the previously identified parity, error-shape, limit-ownership, and dispatch issues have been addressed. A blocking upgrade-compatibility gap remains: protocol 15 applies the native document validator to stored contracts registered under protocol 14 without retaining their legacy validation semantics or enforcing a migration invariant. The signed-zero behavior is documented but still lacks focused boundary coverage, and one token-cost numeric check still rejects integral floating-point values admitted by the legacy meta-schema.
🔴 1 blocking | 🟡 3 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that directly alters protocol-versioned consensus validation and contract-registration acceptance rules in native schema-checking and platform-version dispatch code. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/document_type.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/document_type.rs:312-329: Accept integral floating-point values in token-cost contract IDs
The meta-schema defines each `tokenCost.contractId` byte as an integer, and the native checker generally follows JSON Schema integer semantics where a floating-point value without a fractional part is an integer. This branch instead calls `item.as_u64()`, so a value such as `1.0` is rejected even though the legacy meta-schema accepts it. That creates an additional protocol-15 registration-verdict difference outside the documented exceptions. Use the existing exact numeric helpers, including `is_integer` and the numeric bound comparisons, for these byte values rather than requiring the JSON number to have an unsigned-integer representation.
In `packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs:33-38: Preserve legacy document validation for contracts registered before protocol 15
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891896)
The protocol-15 table selects the native document validator and typed validation-schema builder for all document types, but contracts loaded from state are reconstructed without a registration-generation marker and skip the full-validation registration checks. A contract registered under protocol 14 can therefore have a schema admitted and compiled by the legacy path but rejected by the native builder, such as a property with `$ref` alongside `maxBytes`. When protocol 15 later validates a document for that contract, `validate_document_v2` lazily builds the native validator and returns `InvalidContractStructure`, disabling creates, replaces, and other document operations. The current corpus census only proves that known contracts are representable; it does not establish an invariant for contracts registered during the remaining protocol-14 window. Preserve the validator semantics selected at registration, or add a deterministic migration or activation precondition that makes every pre-15 schema representable before selecting the native validator.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/tests.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/tests.rs:1508-1516: Cover and disclose the signed-zero uniqueness difference
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891902)
The protocol-15 signed-zero rule is now documented, but the registration-check tests still do not exercise the actual divergence from the legacy checker. Add an explicit 16-element unique-list or enum fixture containing `0.0` and `-0.0` and assert the native registration refusal, because the native helper switches from pairwise comparison to its hashed path above 15 elements while the legacy checker can treat the signed-zero bit patterns as distinct. A companion 15-element case can pin the shared pairwise boundary.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/mod.rs:13-16: Cover and disclose the signed-zero uniqueness difference
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4353307235)
The module documentation correctly discloses that protocol 15 treats `0.0` and `-0.0` as one unique-list value, but this implementation-level exception remains unpinned by a focused native-registration regression test. Add a boundary fixture covering the 15-element pairwise path and the 16-element hashed path so changes to numeric normalization or hashing cannot silently alter the documented protocol behavior.
…ma check The document meta-schema checks a `tokenCost` `contractId` with `byteArray`, which reads each byte as an unsigned integer: it admits `1` and refuses `1.0` and `256`, and the native checks agree on all three. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The native schema checker is correctly isolated behind the protocol-version tables, and the previously reported parity, error-shape, limit-placement, and dispatch issues are addressed or refuted. One test-coverage suggestion remains: the documented signed-zero uniqueness difference is not pinned at the 15/16-element boundary. The legacy-contract compatibility concern is real but predates this diff, is not worsened here, and is tracked by #5533; the Kotlin check is still pending and does not cover the changed Rust code.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to consensus-critical document-contract registration and schema validation behavior, including protocol-version dispatch and native validation rules incheck_document_type_schemaandDocumentTypeSchemaVersions. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/tests.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/tests.rs:1508-1516: Cover and disclose the signed-zero uniqueness difference
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891902)
The protocol-15 signed-zero behavior is now documented, but the tests still cover only mixed integer/float precision and do not exercise `0.0` versus `-0.0`. The shared uniqueness implementation has distinct pairwise and long-list hash paths at the 15-element boundary, so a future change could preserve the current numeric cases while regressing signed-zero handling for lists of 15 or 16 elements. Add focused `enum` or `uniqueItems` fixtures at both sizes containing signed zeros and assert the intended native registration and validator results.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Preserve legacy document validation for contracts registered before protocol 15 — The compatibility gap for protocol-14 contracts whose schemas cannot be represented by the protocol-15 native validator is real, but it originates in the protocol-15 document-validator change from #5533 rather than this diff and is not worsened by the native registration checks added here. The author has explicitly tracked the upgrade/migration decision in #5533.
- Follow-up: Resolve the tracked compatibility decision in #5533 before protocol 15 activation, including an upgrade-state test for a pre-15 contract loaded under protocol 15.
…ry length The rule of protocol version 15 holds on both sides of the 15 elements up to which `uniqueItems` compares elements pairwise: a list holding `0.0` and `-0.0` is refused at 2, 15, 16 and 17 elements, and the same list with `0.5` in place of `-0.0` is accepted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-checks Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The registration checks read an `enum`'s members as one unique list with the native document validator's comparison: members holding `0.0` and `-0.0` are refused at 2, 15, 16 and 17 members, and the same members with `0.5` in place of `-0.0` are admitted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The protocol-15 native schema checker is correctly version-gated and the prior rule-parity, error-shape, numeric-comparison, and signed-zero concerns are addressed. Two issues remain: protocol 15 can make previously valid protocol-14 contracts unusable when their schemas cannot build the native validator, and patterns nested under contains use a different regex compilation limit than protocol-14 registration. The Kotlin SDK check is still in progress; no failed code check is reported.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change that alters consensus-critical document-contract schema acceptance and protocol-versioned validation behavior in native Rust code, including functions such as check_document_type_schema and require_validation_schema. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/properties.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/native_schema/properties.rs:114-118: Keep nested `contains` regex compilation aligned with legacy registration
The native registration checker intentionally returns `Ok(())` for `contains`, so it does not apply the 5 MiB `PATTERN_SIZE_LIMIT` to patterns nested inside a `contains` schema. Protocol-14 registration compiles the complete document schema with `RegexOptions { size_limit: Some(5 * (1 << 20)) }`, including such patterns, while the protocol-15 validation-schema builder visits `contains` and compiles its pattern through `validator_regex` with the regex crate's default limit. A pattern whose compiled automaton is between those limits can therefore be rejected during protocol-14 registration but accepted and used by the protocol-15 native validator. Either preserve the 5 MiB limit when building the protocol-15 validator or document and test this additional protocol-15 verdict change.
In `packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs:34-38: Preserve legacy document validation for contracts registered before protocol 15
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891896)
`CONTRACT_VERSIONS_V7` selects the native document validator and typed validation-schema builder for every document type, but stored contracts do not retain the protocol generation under which they were registered. A contract registered under protocol 14 can contain a schema admitted by the protocol-14 meta-schema but rejected by the typed builder, such as a property with `maxBytes` beside a `$ref`; the existing validation-schema tests demonstrate this registration-versus-builder mismatch. After protocol 15 activates, loading the contract skips registration validation, but document operations lazily build the protocol-15 native validator and return `InvalidContractStructure`, disabling creates, replaces, and other writes for that type. Preserve the legacy validator semantics for pre-15 contracts or provide a deterministic activation migration that makes all such schemas representable before selecting native validation. The current corpus census is evidence about existing state, not a protocol guarantee for contracts registered before activation.
…regex limit Through protocol version 14, registration compiled the whole document schema under a 5 MiB regex size limit once the meta-schema check passed, so a `pattern` inside a `contains` or in a system property's schema, which the meta-schema leaves unchecked, had to compile under that limit. The native checks skipped those schemas, and the validation schema's builder compiles patterns under the regex crate's default limit, so a pattern between the two limits registered only at protocol version 15. The native checks now compile those patterns under 5 MiB, down `properties`, `items` and `contains`, and refuse a failing one with the same JSON Schema error. A `$ref` is not followed: the compile reached its target only when a document did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Protocol 15 unconditionally selects the native document validator for all contracts loaded under that protocol, but persisted contracts do not retain the registration-era validation semantics. The existing tests demonstrate that protocol 14 can register a schema with $ref alongside maxBytes that the protocol-15 builder rejects, so document operations for such a contract fail after activation; the current corpus census does not enforce compatibility for future protocol-14 registrations. The Kotlin SDK check is still in progress, but it is unrelated to this DPP change.
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large and intricate change to consensus-critical document-contract registration and protocol-version-gated schema validation, including native rule reimplementation and error-verdict compatibility. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v7.rs:24-38: Preserve legacy document validation for contracts registered before protocol 15
(existing thread: https://github.com/dashpay/platform/pull/5538#discussion_r4366891896)
Protocol 15 selects `validate_document: 2` and `validation_schema: Some(0)` for every contract, while contracts reconstructed from state do not retain the protocol or validator generation under which they were registered. After activation, document validation therefore lazily builds the protocol-15 native validator for legacy contracts as well. The existing `registered_before_validation_schemas` coverage shows that protocol 14 accepts a property schema containing `$ref` together with `maxBytes`, while `document_type_validation_schema` under the latest protocol rejects that same stored schema because the typed builder cannot represent those keywords beside the reference. Such a contract can load successfully after the upgrade but every document create or replace for that type fails with `InvalidContractStructure`. The current corpus census only covers contracts known at the time of the snapshot and does not protect contracts registered under protocol 14 before activation. Preserve the legacy validator semantics for pre-15 contracts, persist or derive a registration-generation marker, or add an enforceable migration/activation gate before selecting the native validator globally.
…perties report The report of an `additionalProperties: false` violation scanned the declared properties for every key of the value. It now collects the declared names into a set once and looks each key up there. The report lists the same keys in the same order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts tests beside it Generation 0 of the validation schema builder parsed `format` through `ValidatorFormat`'s `FromStr`, so a format a later generation added to the enum would also have been admitted by generation 0 at protocol version 15. It now admits the formats it lists by name in `v0`, and the enum no longer parses names. The walk already matches the enum exhaustively. The builder's generation-0 tests move to `validation_schema/v0/tests.rs` and build through `try_from_root_v0`. The parent `tests.rs` keeps what every generation shares: the keyword and type tables, the version dispatch, and the generated schemas and existing-validator helper the builder and walk tests compare with. All 60 tests are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ment validation At protocol version 15, document validation refuses a 16-element list holding both `0.0` and `-0.0` under `uniqueItems`, and accepts 16 distinct numbers, through the native validator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…' into claude/native-registration-checks # Conflicts: # packages/rs-dpp/src/data_contract/document_type/schema/validation_schema/tests.rs
…gistration-checks
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Basic explanation
What this does: When someone registers a data contract, every document type's schema is checked against a "meta-schema", a JSON Schema describing what a valid document type looks like. Today a third-party JSON Schema library (our fork of the
jsonschemacrate) runs that check. From protocol version 15, Platform runs the same rules in its own Rust code instead. Every rule of the meta-schema is ported, with the same verdict and the same error code, but for two deliberate rules listed under Breaking Changes. It also requires every new document type to have a typed validation schema, which its documents are checked against natively (#5533).Value: Contract registration no longer depends on the
jsonschemacrate at protocol version 15. Together with #5533, which checks documents natively, this moves the crate out of new consensus paths, a step toward dropping the fork. Index property names are also held to the 255 bytes a GroveDB key can hold.Risks: Low to medium. The change is consensus-relevant but only at protocol version 15, which is unreleased. Protocol versions up to 14 run exactly the code they ran. The risk is a ported rule that differs from the meta-schema. That is covered by comparing both on every document type ever registered on mainnet and testnet and on about 5,700 generated variants (30,000 in a one-off run), all with equal verdicts. Two kinds of refusal change their error class at 15, listed under Breaking Changes.
Issue being fixed or feature implemented
Step 2 of the 5.1 contract definition work (Refs #5271), stacked on #5533 (native document validation). After #5533, documents of types registered at protocol version 15 are checked natively, but registration still ran the document meta-schema through the
jsonschemafork and compiled every schema with it. This PR removes that last use at registration.What was done?
The schema check, versioned in the tables (
check_document_type_schema, a newDocumentTypeSchemaVersionsfield):jsonschemacrate and compiles it, as before.CONTRACT_VERSIONS_V7only (protocol version 15):native_schema::check_document_type_schemachecks it natively, and once the type is parsed,require_validation_schemarequires the type's native validator to build (DocumentSchemaValidator::try_from_root, the buildervalidation_schemaselects), so a type whose documents protocol version 15 would refuse, such as one whose references cycle without entering the value, is refused at registration.validate_document_type_schemaandrequire_validation_schemadispatch on it with&PlatformVersion. The parser stays generation 3 (try_from_schema3) at protocol version 15, and loading a stored contract runs no schema check.Native checks (
try_from_schema/native_schema/), one file per area of the meta-schema (schema/meta_schemas/document/v3/document-meta.json):mod.rs: the entry points, and the primitives read as the crate read them. A repeated key reads as its last entry, and a float without a fractional part counts as an integer. Bounds compare exactly, aconstnumber matches withinf64::EPSILON, and list uniqueness uses the native document validator's comparison.document_type.rs: the root's closed key set, the document-type keywords and their conditions.properties.rs: property schemas (at every level and in$defs) and typed array element schemas. This covers the closed key sets, identifiers, byte arrays,pattern/formatlengths,encryptedFor,generatedFrom,maxBytesand the rest.indexes.rs: every index keyword and the conditions between them.references.rs:refersTo,ownerRefersTo,creatorRefersTo, with thefindBy,whereandconstraintsgrammars.constraints.rs: the rule language ofpropertyConstraintsanddeleteConstraints.A refusal is the
JsonSchemaError(code 10101) the meta-schema check returned, with the same keyword, location (instance_path), parameters and property name. Two fields differ: the message text, andschema_path, which is empty (as is theschemaPathnested in a property name's error), since the native checks have no location in the meta-schema. All of it goes toinfo, which is not part of the results hash.The port reproduces the meta-schema's JSON Schema semantics where they matter. For example, its
ifpasses when the key it tests is absent, so a property schema with neithertypenor$refmust declarepropertiesandadditionalProperties. A failedanyOf/oneOfis reported at the value. A property'scontainsand a system property's schema ($createdAtand the rest) are not checked against the meta-schema, but eachpatternin them must compile under the 5 MiB regex size limit, because through protocol version 14 thejsonschemacrate compiled the whole schema under that limit once the meta-schema check passed. That refusal reports the pattern's location ininstance_path, where the crate's compile error put it inschema_path; both go toinfo.One new rule: an index property name is at most 255 bytes, the longest key GroveDB stores. The meta-schema admitted 256 characters.
Validation schema builder: a plain-name
$idmay also contain$, asrefersTodata carries ("$id": "$ownerId"). The builder only runs at protocol version 15.The meta-schema's numbers and the cap are constants of the native checks, beside the rules that read them, as the meta-schema JSON held them before. They are not
SystemLimits: they belong tocheck_document_type_schema1 alone.Before and after
A document type with a null flag:
{ "type": "object", "documentsMutable": null, "properties": { "name": { "type": "string", "maxLength": 63, "position": 0 } }, "additionalProperties": false }jsonschemacrate,JsonSchemaError(10101) at/documentsMutable.JsonSchemaError(10101) at/documentsMutable.An index on a property named with 128 two-byte characters (256 bytes):
JsonSchemaError(10101) at/indices/0/properties/0: "the index property name … is longer than 255 bytes".A document type the validation schema does not represent, for example
maxBytesbeside a property's$ref, which document validation never checked:InvalidContractStructure(10231): "maxBytesis read beside a property's$ref…".How Has This Been Tested?
native_schema/tests.rscompares the native checks with the meta-schema, and registration at the latest protocol version withcheck_document_type_schema1 against the same with 0:tests/fixtures/contract_corpus): same verdict, same parsed document type;retractedWhen, constraints, windowed and ranked indexes, index-only types;rs-drive,rs-drive-abciandrs-dppand every system contract (207 contracts) registered at protocol versions 14 and 15: same verdict and code.validation_schematests: types the builder refuses now register at protocol version 14, and protocol version 15 refuses them.cargo test -p dpp --lib data_contract(3,118),state_transition(1,270),--test contract_corpus;cargo clippy -p dpp -p platform-version --all-targets --features dpp/all_features;cargo check -p dppwithvalidation-schema,validation,state-transitionsandjson-schema-validationalone.Breaking Changes
None for released protocol versions. At protocol version 15 (unreleased):
InvalidContractStructure).jsonschemacrate failed to compile, refused before withJsonSchemaError, is now refused by the validation schema builder (InvalidContractStructure).enum's members,0.0and-0.0are one value however long the list, as in feat(dpp)!: validate documents natively at protocol version 15 #5533.In-place changes to shipped generations
DocumentTypeSchemaVersionsgainscheck_document_type_schema, 0 in every shipped table (CONTRACT_VERSIONS_V1toV6).validate_document_type_schemadispatches on it, and 0 runsvalidate_against_meta_schema_and_compileexactly as before.require_validation_schemalast under full validation, which returns at once under 0.SystemLimitsgainsmax_index_property_name_bytes, set to 255 in every shipped table (SYSTEM_LIMITS_V1toV4, and the test mock). Onlycheck_document_type_schema1 (protocol version 15) reads it; the shipped versions, under 0, never do.validator_regexnow builds throughvalidator_regex_within(pattern, None), the same compile with the crate's default size limit. The builder and the native document validator run only at protocol version 15.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
782ec00/skip-botsproceeds without the ones not yet reported/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.